Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

additional Build-System features & fixes #327

Merged
merged 14 commits into from
Aug 29, 2017

Conversation

drywolf
Copy link
Contributor

@drywolf drywolf commented Aug 2, 2017

Features

  • support for multiple build virtualization options per platform (Docker & Vagrant)
    • added Vagrant build support for the win32 target (should help with this requirement)
  • support for customizing platform builds for different OS vendors
  • added nodejs.py utility script to package / manage the node.js code / binaries
  • added documentation headers & some more inline documentation for most py files
  • new --no-shutdown option for vagrant builds (keeps the used Vagrant VM running after a build is finished)
  • new --keep-native-libs option provides basic support for including different native libraries into a single JAR bundle
  • added an interactive mode for the build.py CLI
  • added support for "anti-steps" when running a build
    • e.g. all ~nodejs ~test runs all build-steps except for nodejs and test
  • documented new build-system features in BUILDING.md
  • split PlatformDetection code from LibraryLoading code & tried to use more normalized strings for platforms, vendors and architectures
  • updated & added some tests
  • when the Android emulator is running within Docker it now writes a logcat.txt file for debugging of the emulator status & potential problems

Fixes

- moved check_node_builtins to build_utils
- added --no-shutdown option for vagrant builds
- fixed typo in JNI lib output name on win32 (missing lib prefix)
- refactored all existing platform configs to align with new build-env APIs
- added graceful handling / shutdown of build-envs if user cancels the build early
- added powershell scripts for build dependencies
- corrected usage of "smb" mounted folders for macos:vagrant target
- added win32:vagrant target
- register atexit events before cross-builds are started
- switched linux & android docker images to Ubuntu Xenial LTS (see eclipsesource#311)
- cleaned up and documented switch-to-winrm-plugin that is used in win32:vagrant builds
Features
------------------------
[Build-System]
- replaced default Android instrumentation test runner with Spoon test runner
  (allows to optionally run individual tests on emulator)
- added interactive CLI mode (see build.py, build_interactive.py)
- added "--interactive, -i" CLI parameter to start interactive build CLI
- added support for specifying "anti-build-steps" via the CLI
  (for example: "all ~nodejs ~test" runs all steps, except for "nodejs" and "test")
- added "--vendor, -v" CLI parameter that allows to build specialized linux vendor binaries, e.g. for alpine-linux
- added alpine-linux binary support
- added "inject_env" API that is directly callable on a build-step config
- added "--sys-image, -img" CLI parameter that can be used to override the used OS image for Docker / Vagrant builds
- refactored the original "--cross-compile, -x" parameter into two separate parameters "--docker, -dkr" and "--vagrant, -vgr"
  (this makes it possible to support both variants for all target platforms)
- added "--keep-native-libs, -knl" CLI parameter
  (can be useful for building bundles that should include multiple platform binaries)
- centralized some repeated constant literals & code from build-steps across multiple platforms
  (into build_utils.py, cmake_utils.py and shared_build_steps.py)
- added automatically set "file_abi" property on build-step configs
- added more variables for use with "inject_env" API
- added "adb logcat" process to Android Docker build
  (writes realtime output from Android emulator to ./docker/android/logcat.txt)
- moved all Node.js utility code to nodejs.py CLI script
- added experimental code to package Node.js pre-built binary packages
  (see nodejs.py)

[J2V8]
- updated LibraryLoader code to include Linux-Vendor specifier when looking for native libs
  (vendor-specific libs are preferred over vendor-agnostic ones)
- added PlatformDetector as a central place to get normalized string identifiers for OS,Arch,Vendor

Fixes & Changes
------------------------
[Build-System]
- reorganized some code of the build-system into more separate files
  (see build.py, build_constants.py, build_executor.py, cli.py)
- added missing "bool" type to immutable.py
- throw error in CMake scripts if Java-Home could not be found
- default Docker base-image for Linux & Android builds is now "debian:jessie"
- installation of JDK in install.jdk.sh now checks if a JDK is already installed
  (needed for alpine-linux build)
- refactored win32 Dockerfile
  (now uses the same PowerShell scripts as the Vagrant build instead of inline commands)

[J2V8]
- separated LibraryLoader and Platform-Detection code
  (see PlatformDetector.java)
- show native lib-name in IllegalStateException message when native lib could not be loaded
- AllTests.java is now called A_RunAheadTests and only includes the V8RuntimeNotLoadedTest
  (this is the only test that should be run before any other tests)
- extended and refactored LibraryLoaderTest / PlatformDetectorTest implementations
- added timeout for V8LockerTest
- added workaround for V8RuntimeNotLoadedTest (is currently not implemented to run on Android)
- added build-util that directly sets the required build variables into the pom.xml file without using environment variables
- reverted gradle sdk- & target-versions back from 19 to 10
- added alias-functions for the polyfill shell commands to shared_build_steps.py
- started documenting the build-system modules & functions
- fixed git EOL problems for shell/python scripts used by Docker builds
@drywolf
Copy link
Contributor Author

drywolf commented Aug 2, 2017

@matiwinnetou As promised 😉
@irbull

  • please let me know if something needs work or explaination
  • this PR includes some Java code from os-maven-plugin
    • see PlatformDetector.java
    • I already talked about the licensing attribution for this code a bit with @trustin
    • the PlatformDetector.java file now additionally contains the Apache License for the orginal code as well as the J2V8 Eclipse license for any other parts
    • @irbull @trustin please let me know if that's alright for the both of you

My Roadmap for when this PR is done

  1. I would continue work on getting a fully automated CI integration going (nodejs packages, travis, etc. ... to further improve upon Decide on binary release strategy #261)
  2. I would start cleaning up the repository from the now redundant and partially already outdated shell scripts. I thought about doing it in this PR already, but I think it is better to review & get in the new stuff first and then clean up the deprecated parts in a separate PR later.

BUILDING.md Outdated

usage: build.py [-h] --target {android,linux,macos,win32} --arch {x86,x64,arm}
[--vendor VENDOR] [--keep-native-libs] [--node-enabled]
[--docker] [--vagrant] [--sys-image SYS_IMAGE] [--no-shutdown]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see -v in the list of description here...

Copy link
Contributor Author

@drywolf drywolf Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-v is just the short form for the --vendor switch, I haven't yet seen a way to show both in the python args parser generated help

BUILDING.md Outdated
[0] Docker >> android-x86 >> NODE_ENABLED
[1] Docker >> android-arm >> NODE_ENABLED
[2] Docker >> alpine-linux-x64 >> NODE_ENABLED
[3] Docker >> linux-x64 >> NODE_ENABLED
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose these ones as suggestions? Is it possible to add suggestions? I though interactive would be that I would pick target, vendor, platform, etc, interactively.. also I found a tiny bug, it is -i or --interactive, --i doesn't work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you choose these ones as suggestions?
I though interactive would be that I would pick target, vendor, platform, etc, interactively..

Because not all combinations of platforms * architectures * vendors * virtualization-tech * feature-variations[N] are currently technically possible nor would it be sensible to try and support them all.

For the current suggestions I added the ones that are working and that were important for me to test the cross-platform build.

Is it possible to add suggestions?

Yes, have a look at build_system/build_configs.py

also I found a tiny bug, it is -i or --interactive, --i doesn't work

You can use -i or --interactive ... they are both doing the same thing (they are aliases)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That being said, I also can imagine many ways to improve upon this feature and see what would be the best DX for the interactive CLI for devs that want to build J2V8 themselves.
But that would be stuff for a next PR ... this one is already bloated enough 😆

BUILDING.md Outdated
[build-steps [build-steps ...]]
```
```
python build.py -v alpine -t linux -a x64 -dkr -img openjdk:8u131-alpine -ne j2v8
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it sounds silly but I would suggest to put more examples here for poor souls that try to understand params.

BUILDING.md Outdated
# Build-Steps

The J2V8 build-system performs several build steps in a fixed order to produce the final J2V8 packages for usage on the designated target platforms. What follows is a short summary for what each of the executed build-steps does and what output artifacts are produced by each step.

---
## Node.js

Builds the [Node.js](https://nodejs.org/en/) & [V8](https://developers.google.com/v8/) dependency artifacts that are later linked against by the J2V8 native bridge code.
Builds the [Node.js](https://nodejs.org/en/) & [V8](https://developers.google.com/v8/) dependency artifacts that are later linked into the J2V8 native bridge code.
Copy link
Contributor

@matiwinnetou matiwinnetou Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel guilty about commenting that you did so much work but I have to say I didn't learn much from description from nodejs.py:

➜  J2V8 git:(drywolf-build-sys-features) ✗ python nodejs.py -h
usage: nodejs.py [-h] command

positional arguments:
  command

optional arguments:
  -h, --help  show this help message and exit

I still don't know how checkout nodejs source, it used to be python prepare_build.py

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to breat something to get commands:

➜  J2V8 git:(drywolf-build-sys-features) ✗ python nodejs.py 7.4.0
usage: nodejs.py [-h] command
nodejs.py: error: argument command: invalid choice: '7.4.0' (choose from 'flush-cache', 'fc', 'git-init', 'gi', 'package', 'pkg', 'store-diff', 'sd', 'apply-diff', 'ad')

Copy link
Contributor Author

@drywolf drywolf Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is far from finished, I need to first get a better picture of how this would be used in a CI build before I start this kind of polishing 😉
But I agree that this help output is pretty useless even at this point, I will add some preliminary help for now.

PS: Thanks for reviewing ... once you are done I will start to see what needs fixing and how to prioritze 👍

PPS: looks like there is a feature of the args parser that I could use to improve upon that

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@matiwinnetou
Copy link
Contributor

matiwinnetou commented Aug 2, 2017

I like store patch in nodejs.py little helper but not really sure what package function does, when would one use it?

I am trying to do some QA here as well:
On OSX this is what I used to build:

python build.py --target macos --arch x64

It worked, except for unit tests

Tests run: 18, Failures: 0, Errors: 18, Skipped: 0, Time elapsed: 0.399 sec <<< FAILURE! - in com.eclipsesource.v8.NodeJSTest
testSingleThreadAccess_HandleMessage(com.eclipsesource.v8.NodeJSTest)  Time elapsed: 0.152 sec  <<< ERROR!
java.lang.UnsupportedOperationException: StartNodeJS Not Supported.
	at com.eclipsesource.v8.NodeJSTest.setup(NodeJSTest.java:31)

testSingleThreadAccess_HandleMessage(com.eclipsesource.v8.NodeJSTest)  Time elapsed: 0.152 sec  <<< ERROR!
java.lang.NullPointerException
	at com.eclipsesource.v8.NodeJSTest.tearDown(NodeJSTest.java:36)

V8Runtimes not properly released
V8Runtimes not properly released
V8Runtimes not properly released
V8Runtimes not properly released
V8Runtimes not properly released
V8Runtimes not properly released
V8Runtimes not properly released

I think NodeJS unit tests failed because I didn't build this with NodeJS support?

It produced jar file:

j2v8_macos_x86_64-4.8.0-SNAPSHOT.jar

though

nodejs.py Outdated
import os
import sys
import tarfile
import zipfile
Copy link
Contributor

@matiwinnetou matiwinnetou Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is showing in my IDE as not needed..., but maybe it is...

import argparse
import collections
import fnmatch
import glob
Copy link
Contributor

@matiwinnetou matiwinnetou Aug 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is showing in my IDE as not needed..., but maybe it is...

@matiwinnetou
Copy link
Contributor

Dockerized build for linux didn't work for me:

python build.py -t linux -a x64 -dkr -img ubuntu:zesty j2v8

There is NO WARRANTY, to the extent permitted by law.
Updating Maven configuration (./docker/shared/pom.xml)...
SHELL building linux@x64 => j2v8jni
Scanning dependencies of target j2v8
make[2]: *** No rule to make target '../../node/out/Release/obj.target/deps/uv/libuv.a', needed by 'libj2v8-linux-x86_64.so'.  Stop.
make[2]: *** Waiting for unfinished jobs....
[ 50%] Building CXX object CMakeFiles/j2v8.dir/jni/com_eclipsesource_v8_V8Impl.cpp.o
CMakeFiles/Makefile2:67: recipe for target 'CMakeFiles/j2v8.dir/all' failed
make[1]: *** [CMakeFiles/j2v8.dir/all] Error 2
make: *** [all] Error 2

@matiwinnetou
Copy link
Contributor

matiwinnetou commented Aug 2, 2017

ON OSX I was able to run a docker build for linux but it fails now due to some permission issue that I didn't have before:

➜  J2V8 git:(drywolf-build-sys-features) ✗ python build.py -v alpine -t linux -a x64 -dkr -img openjdk:8u131-alpine -ne j2v8 
Checking Node.js builtins integration consistency...
Caching Node.js artifacts...
>>> Node.js build-cache used: alpine-linux.x64
CONTAINER           CPU %               MEM USAGE / LIMIT   MEM %               NET I/O             BLOCK I/O           PIDS
Error response from daemon: No such container: j2v8.alpine.linux.x64
Updating Maven configuration (./docker/shared/pom.xml)...
preparing linux@x64 => docker-build-host
Building docker image: j2v8-alpine-linux
Sending build context to Docker daemon  56.83kB
Step 1/18 : ARG sys_image=debian:jessie
 ---> 
Step 2/18 : FROM $sys_image
 ---> 478bf389b75b
Step 3/18 : ARG vendor=debian
 ---> Using cache
 ---> e7e46ab65d53
Step 4/18 : RUN mkdir -p /temp/docker/shared/
 ---> Using cache
 ---> f993c095007a
Step 5/18 : WORKDIR /temp/docker/shared/
 ---> Using cache
 ---> b81f13c47bb5
Step 6/18 : COPY ./shared/install.${vendor}.packages.sh /temp/docker/shared
 ---> Using cache
 ---> dd608e3556ce
Step 7/18 : RUN ./install.${vendor}.packages.sh
 ---> Running in d9fff725a389
/bin/sh: ./install.alpine.packages.sh: Permission denied
The command '/bin/sh -c ./install.${vendor}.packages.sh' returned a non-zero code: 126
Traceback (most recent call last):
  File "build.py", line 18, in <module>
    bex.execute_build(args)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_executor.py", line 221, in execute_build
    execute_build_step(cross_compiler, cross_cfg)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_executor.py", line 190, in execute_build_step
    build_system.build(build_step)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_structures.py", line 82, in build
    self.pre_build(config)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/docker_build.py", line 84, in pre_build
    self.exec_host_cmd("docker build " + args_str + " -f $PLATFORM/Dockerfile -t \"" + image_name + "\" .", config)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_structures.py", line 88, in exec_host_cmd
    self.__exec_cmd_core(cmd, config, config.host_cwd)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_structures.py", line 105, in __exec_cmd_core
    utils.execute(cmd, cwd)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_utils.py", line 52, in execute
    raise subprocess.CalledProcessError(return_code, cmd)
subprocess.CalledProcessError: Command 'docker build  --build-arg sys_image=openjdk:8u131-alpine --build-arg vendor=alpine -f linux/Dockerfile -t "j2v8-alpine-linux" .' returned non-zero exit status 126
Waiting for docker process to exit...
Error response from daemon: No such container: j2v8.alpine.linux.x64
Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/docker_build.py", line 69, in cli_exit_event
    self.exec_host_cmd(docker_stop_str, config)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_structures.py", line 88, in exec_host_cmd
    self.__exec_cmd_core(cmd, config, config.host_cwd)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_structures.py", line 105, in __exec_cmd_core
    utils.execute(cmd, cwd)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_utils.py", line 52, in execute
    raise subprocess.CalledProcessError(return_code, cmd)
CalledProcessError: Command 'docker stop j2v8.alpine.linux.x64' returned non-zero exit status 1
Error in sys.exitfunc:
Traceback (most recent call last):
  File "/System/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/atexit.py", line 24, in _run_exitfuncs
    func(*targs, **kargs)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/docker_build.py", line 69, in cli_exit_event
    self.exec_host_cmd(docker_stop_str, config)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_structures.py", line 88, in exec_host_cmd
    self.__exec_cmd_core(cmd, config, config.host_cwd)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_structures.py", line 105, in __exec_cmd_core
    utils.execute(cmd, cwd)
  File "/Users/mati/Devel/OpenSource/J2V8/build_system/build_utils.py", line 52, in execute
    raise subprocess.CalledProcessError(return_code, cmd)
subprocess.CalledProcessError: Command 'docker stop j2v8.alpine.linux.x64' returned non-zero exit status 1
➜  J2V8 git:(drywolf-build-sys-features) ✗ 

@drywolf
Copy link
Contributor Author

drywolf commented Aug 2, 2017

Dockerized build for linux didn't work for me:

python build.py -t linux -a x64 -dkr -img ubuntu:zesty j2v8

Did you build nodejs before that ? the error says that it cant find the nodejs static libraries that j2v8 wants to link to. This is because specifying only j2v8 will not build nodejs.

@matiwinnetou
Copy link
Contributor

I found one culprit I think:

-rw-r--r--   1 mati  staff   185  2 sie 21:07 install.alpine.packages.sh

this does not have executive rights

@matiwinnetou
Copy link
Contributor

@drywolf how am I supposed to build node, do you have helpers for this? When I only use J2V8 without NodeJS but only V8 am I still obliged to build NodeJS because it has dependencies in to V8 deps folder?

You mentioned there is a way to build against nodejs version that was downloaded and already build for the platform, how does one proceed with that? The more I dig into the topic the more I see what a complicated and big fish we are trying to cook here. There are many variations...

@drywolf
Copy link
Contributor Author

drywolf commented Aug 2, 2017

this does not have executive rights

Yeah, IIRC we already had this issue before for the other shell scripts ... it is kind of annoying that there seems to be no way to tell git that execute permissions should be always given to .sh scripts in a repo. 😕

PS: we are not the only ones feeling this pain

@drywolf
Copy link
Contributor Author

drywolf commented Aug 2, 2017

how am I supposed to build node, do you have helpers for this? When I only use J2V8 without NodeJS but only V8 am I still obliged to build NodeJS because it has dependencies in to V8 deps folder?

As far as I know it will always be NodeJS that is used to get a linkable set of static V8 libraries and the additional Node.js static libraries (libuv, etc.) ... please correct me if I am wrong @irbull , but I have not yet seen any indication that J2V8 was ever linked against binaries produced by building V8 from the original V8 code

So in a sense if you want to link only against the V8 libs produced by a Node.js build that is fine. J2V8 is just piggybacking onto the Node.js code because from Node.js you get both options for free instead of having to support two different third party dependencies (V8 and Node.js independently).

You mentioned there is a way to build against nodejs version that was downloaded and already build for the platform, how does one proceed with that?

That's the end goal ... right now you can build node.js via the build.py CLI and then run the nodejs.py package command to get a zipped archive. This could be hosted online and later downloaded for a build that does not need to build node.js/v8 anymore but will only build the j2v8 parts

The more I dig into the topic the more I see what a complicated and big fish we are trying to cook here. There are many variations...

Yeah, that's also why the question posed in #261 is such a tricky one ... how to decide what is the set of binaries that this project should provide
platforms * architectures * vendors * virtualization-tech * feature-variations[N] = 😱

@drywolf
Copy link
Contributor Author

drywolf commented Aug 2, 2017

When I only use J2V8 without NodeJS but only V8 am I still obliged to build NodeJS because it has dependencies in to V8 deps folder?

But you still have a important point there ... I am not sure how all the different linkers will handle this ... right now I assumed that if NODE_COMPATIBLE=0 is given to a compiler/linker for the J2V8 native build that the unused static libraries will not be included by the linker anyway (since no C++ of J2V8 should then touch them). But this could be a wrong assumption 🤷‍♂️

To make perfectly sure that no Node.js specific libs are ever linked when node-enabled is not active, I would have to conditionally leave out the extra Node.js libs (libuv, etc.) in the CMake scripts.

pom.xml Outdated
<modelVersion>4.0.0</modelVersion>
<properties>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
<os>${env.J2V8_PLATFORM_NAME}</os>
<os>alpine-linux</os>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct? Why do we assume in pom file some alpine-linux?

Copy link
Contributor Author

@drywolf drywolf Aug 4, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok ... this string will be set to the correct value by the build-system before the java build step is running. It performs a manipulation of the XML directly, see #320 for why this should be a good change to make.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but if this will change why this is alpine-linux by default? This is one of those things that affects the current build process right, the one which @irbull currently is running parallel to the new one (authored by you)

Copy link
Contributor Author

@drywolf drywolf Aug 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats true, but that's also how it was before any changes I made ... there was also some current settings in the POM that were merely there because it was the last thing that was built and coincidentially I guess this was what ended up being committed to the git repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will put some XML comments in front of those values, then it should be clear that those are only temporary values

@matiwinnetou
Copy link
Contributor

@drywolf

I may have already asked this question but I really don't get it.

Lets say I wanna use "j2v8 alias"

  • CLIStep(c.build_j2v8, " Run all build steps to build J2V8 (this does not try to build Node.js)\n" +
  •                  "                This is useful when building with a pre-compiled Node.js dependency package."),
    

This says that it requires NodeJS dependency to be prebuilt. What does it mean? Does it mean I can download it from the Internet for my platform, put into some directory and then invoke build with j2v8 alias?

@matiwinnetou
Copy link
Contributor

Is there any way we can prevent NodeJS unit tests from running? This is just another failure scenario possibility. NodeJS tests should be run within NodeJS project, they should work there not on our machines/jenkins.

@matiwinnetou
Copy link
Contributor

matiwinnetou commented Aug 3, 2017

@drywolf I tested at least docker build with linux, debian/jessie and native osx build. Both of them work, for my needs at the moment this is LGTM but obviously there would have to be lots of test cases...

maybe somebody can test android, alpine linux scenario, etc

@matiwinnetou
Copy link
Contributor

matiwinnetou commented Aug 3, 2017

I managed to built successfully with NodeJS 7.9.0:

You could add this in this PR to J2V8/node.patches folder.
patches.tar.gz

Sadly 7.10.x and 8.x series does not work due to:
nodejs/node#13500

@drywolf
Copy link
Contributor Author

drywolf commented Aug 4, 2017

This says that it requires NodeJS dependency to be prebuilt. What does it mean? Does it mean I can download it from the Internet for my platform, put into some directory and then invoke build with j2v8 alias?

To clarify...
What I do not mean by "prebuilt" / "prebuilding" is anything that can be obtained anywhere from the Node.js download site. There you can just get some prebuilt executable/installable Node.js packages.

But they are of no use to the J2V8 project, because we need to have the intermediate development files (object files .o, static libraries, .a / .lib), because that is what is needed to compile & link the J2V8 JNI C++ interop code.
(I don't think you can get those files anywhere on the internet ... but if you know a place, please let me know 😁)

What I am working on with the build-system / CI process is the following (just a rough outline here):

2 Roles

  • [J2V8 maintainer]
  • [J2V8 user]

[J2V8 maintainer]

  1. has checked out the J2V8 git repo
  2. has checked out the Nodejs git repo (and put it into the J2V8/node dir)
  3. runs python build.py ... nodejs for all platforms and architectures that should be officially supported
  4. runs python nodejs.py package ... this will take all the nodejs object-files and static-library files that were produced in step 3) and put them into a tar.bz2 compressed archive at the J2V8 root directory
  5. this is the dependency package that the maintainer can upload to (e.g.) github and share with other devs or use in a CI build

[J2V8 user]

  1. clones the J2V8 git repo
  2. downloads the dependency package created by the maintainer
  3. extracts it into the J2V8 directory on their machine
  4. runs python build.py ... j2v8 for any platform/architecture that is supported
  5. they have successfully produced a working J2V8 jar that they can customize and use in their project(s)

@drywolf
Copy link
Contributor Author

drywolf commented Aug 4, 2017

Is there any way we can prevent NodeJS unit tests from running? This is just another failure scenario possibility. NodeJS tests should be run within NodeJS project, they should work there not on our machines/jenkins.

Is this about the same thing as you mentioned above ?

I think NodeJS unit tests failed because I didn't build this with NodeJS support?

That is right, and those are not just "NodeJS" tests ... those are J2V8's tests to ensure proper integration of NodeJS when it is enabled.

So I can't quite agree that those are inappropriate to run in J2V8 nor do those tests have any relevance for the actual Node.js project ... forgive me if I am overlooking something here, are there any actual Node.js tests that are running when Node.js is being built ? I have not yet seen anything that would look like it ... but please let me know if you know something that I don't 😉

PS: I already have a working solution that I implemented yesterday that will not run the J2V8 NodeJS tests if the build does not have node-enabled specified. I will add it to the PR once I have time to test it some more and look into the other things that came up in your review 👍

@drywolf
Copy link
Contributor Author

drywolf commented Aug 16, 2017

What where the build params / the command line that you used to build ?
Also please try to run a clean build, i.e. remove the node and node.out directories to make sure nothing is linked in from an older version potentially.

I don't understand why normal build tinkers with *.h files...

What do you mean ?

@matiwinnetou
Copy link
Contributor

matiwinnetou commented Aug 16, 2017

I will try to compile again on OSX and Linux with ensure I cleaned up all stuff (I think I did).

Keep in mind I compiled on OSX natively instead of vagrant...

@drywolf
Copy link
Contributor Author

drywolf commented Aug 16, 2017

Yeah, will be interesting to see if it works from a clean build. If it does then it might have to do with some environmental difference between the Vagrant MacOSX box and the environment on your native MacOSX box.

I just saw that the static flag is missing from the Win32 build options for Node.js ... I am retrying this one right now.

PS: also I didn't catch your changes to the J2V8 cpp file in your fork, I am retrying now with those changes in place.

@matiwinnetou
Copy link
Contributor

matiwinnetou commented Aug 16, 2017

My unit tests fail on Manjaro Linux on a fresh NodeJS enabled build on linux (no docker) and 8.2.1 NodeJS

Run 1: V8Test.seutp:43 » IllegalState J2V8 native library not loaded (j2v8-manjaro-li...
[ERROR]   Run 2: V8Test.tearDown:49 NullPointer
[INFO] 
python build.py -t linux -a x86 --node-enabled all

As you can see it claims to be in vendor manajro mode.

[mati@wit J2V8]$  git:(𝘮 ← 10 nodejs8) 1≡ 1Mcat /etc/issue
Manjaro Linux \r  (\n) (\l)

I guess this is minor issue because who will run j2v8 in production on Manjaro linux... but it shows that it is trickier than one thinks with LibraryLoader.java

Using this command and NodeJS 8.2.1 everything builds fine:

python build.py -t linux -a x86 --node-enabled all ~test

@matiwinnetou
Copy link
Contributor

@drywolf I added --enable-static to 8.2.1 diff patch for Windows build.

@matiwinnetou
Copy link
Contributor

matiwinnetou commented Aug 16, 2017

@irbull perhaps you may reconsider #285, I am talking partially to @drywolf about a topic that we could really handle outside of this PR... Update to NodeJS 8.2.1. Normally I would use chat room for this.

@drywolf
Copy link
Contributor Author

drywolf commented Aug 16, 2017

Thx, but with the more recent node versions the static flag can be passed to the Node.js vcbuild.bat and hence we can skip patching this in the files.

Status update from my side ...
With your cpp changes + the static flag, win32 is not build and unit tests are running ok, except for the unit tests in the v8/debug/ folder ... I guess the debugging interfaces for V8 have changed ?!

I'm now going to re-try MacOS and Linux...

PS @matiwinnetou I also thought the same about communicating ... maybe we can connect via some other IM in the meantime .. do you use Skype ?

- we must use atexit to stop Vagrant VMs / Docker containers in case of an error
@drywolf
Copy link
Contributor Author

drywolf commented Aug 17, 2017

With the tests in v8/debug/ disabled and the additions from @matiwinnetou I was now able to build & run tests successfully for Node.js v8.2.1 on:

  • Android (needs fixing, see this)
  • Alpine Linux
  • Linux
  • MacOS
  • Windows

By looking into android-configure and the Android NDK docs I hope it will be not too hard to get Android working also 🤞

@matiwinnetou
Copy link
Contributor

I use Skype rarely but I can change this for this project, my SkypeId: mati22081979, please add me anybody who wishes to communicate on this project. We can create some team chat there. I would prefer gitter.im but AFAIK I cannot create Gitter chatroom as I am not a project maintainer...

@aschrijver
Copy link

aschrijver commented Aug 17, 2017

@matiwinnetou You can create a freenode channel, e.g. #j2v8 and it is also possible to keep a freenode channel in sync with gitter and vice versa. See for example: http://webchat.freenode.net/?channels=dat

See: http://freenode.net/news/registering-a-channel-on-freenode

Freenode is nice and simple and accessible to anyone.

@matiwinnetou
Copy link
Contributor

@drywolf I changed my mind, can you remove from this PR all patches to NodeJS except for 7.4.0. We should try to keep this PR only related to build system. My PR regarding NodeJS 8.2.1 upgrade will contain a new file in node.patches folder...

message("J2V8_LIB_PLATFORM_NAME = ${J2V8_LIB_PLATFORM_NAME}")
message("J2V8_TARGET_ARCH = ${J2V8_TARGET_ARCH}")
message("J2V8_BUILD_X64 = ${J2V8_BUILD_X64}")
message("--------------------------------------------------")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One thing that I am missing, that could be either here or in a 'success' message at the end, is the V8 and NodeJS versions that have been compiled into the final output (e.g. the .aar).

They are not user-selectable versions, but having them hard-coded makes it more important to report what you'll end up with after build completes.

Copy link
Contributor Author

@drywolf drywolf Aug 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I will try to include this information even earlier in the build process, so a user can abort as early as possible if he meant to build something else.
I am not sure though if this information should always be displayed or if it should be only displayed when an extra command line switch is present or some kind of verbosity level is set.

I am thinking about this, because now that most features that I had planned are done, I am trying to think about the Developer-Experience and one part of that is the verbosity of the build output ... there is already a lot of stuff happening if one runs a complete build for a platform with all the build-steps involved. Two lines for the Node.js/V8 versions of course won't make the big difference there, but at least I wanted to let everyone know that I am thinking about this aspect now also.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would go with a message right at the start. Let's hit those devs with those 2 lines :D

@drywolf
Copy link
Contributor Author

drywolf commented Aug 17, 2017

We should try to keep this PR only related to build system.

That's also what I was thinking

@aschrijver
Copy link

aschrijver commented Aug 17, 2017

@matiwinnetou did you see my freenode suggestion in all the comments floods on this PR?

I added that and Gitter usage to the future direction discussion thread.

@irbull
Copy link
Member

irbull commented Aug 17, 2017

I've opened #331 for the chat request.

- improve merging of pre-defined and user build-params for `build -i`
@irbull
Copy link
Member

irbull commented Aug 28, 2017

Has anyone had luck with this PR and native builds on MacOS. All the tests are failing for me with an unsatisfied link error. The build looks like it works, so I assume it's just the changes to the library loader. I'm trying to debug it now

@irbull
Copy link
Member

irbull commented Aug 28, 2017

I added some debug statements to the native code, and now the build is running fine...
hum... maybe there was a timestamp issue with a file.

@irbull
Copy link
Member

irbull commented Aug 28, 2017

Builds fine locally. I'm trying to get our CI builds working with this setup. Travis seems to have problems with recent versions of cmake.

Copy link
Member

@irbull irbull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is amazing! I haven't gone through all the different configurations yet, but it works locally and I managed to update the travis build to use this. Let's get this in and fix any subsequent issues in future commits.

@irbull
Copy link
Member

irbull commented Aug 29, 2017

Do you still use the docker files in /docker? @drywolf

@irbull irbull merged commit 0e83e34 into eclipsesource:master Aug 29, 2017
@drywolf
Copy link
Contributor Author

drywolf commented Aug 29, 2017

@irbull Dockerfile.android and Dockerfile.linux are not used by my code.
Those are among the files that I was already talking about in a previous comment, which should be cleaned up now to make the transition complete from the previous build scripts & files to the new system.

@irbull
Copy link
Member

irbull commented Aug 29, 2017 via email

@aschrijver
Copy link

congratulations everyone working on this PR, especially @drywolf 🎉 🎈 🎆

this is great stuff, great quality!

i am now busy working on my non-profit movement for people who care, innercircles
but I'm sure I'll be back later (like the other Arnold once said) :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants